-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
test(react-start): benchmarking setup w/ platformatic/flame and autocannon #6457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds a new end-to-end flamegraph benchmarking project at e2e/react-start/flamegraph-bench with React Start app scaffolding, routing, client/server entries, Vite/TS configs, benchmark and server scripts, and .gitignore rules for profiling artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant B as "Benchmark (autocannon)"
participant E as "Express test server\n(dist wrapper)"
participant N as "Node handler\n(toNodeHandler)"
participant S as "React Start server\n(handler.fetch)"
participant R as "Router / SSR\n(routeTree)"
participant FS as "Static assets\n(dist/client)"
rect rgba(200,200,255,0.5)
B->>E: HTTP requests (random /page/:id)
end
rect rgba(200,255,200,0.5)
E->>N: forward request
N->>S: call handler.fetch(request)
S->>R: render route (SSR)
R-->>S: rendered HTML
S-->>N: response
N-->>E: response
E-->>B: response (HTML or asset)
end
opt static asset hit
E->>FS: serve static file
FS-->>E: file bytes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit ad7a1c3
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@e2e/react-start/flamegraph-bench/package.json`:
- Around line 13-14: Replace the internal workspace dependency versions that
currently use the caret protocol with the workspace wildcard protocol: update
the entries for "@tanstack/react-router" and "@tanstack/react-start" in
package.json to use "workspace:*" instead of "workspace:^" so they conform to
the internal dependency guideline.
In `@e2e/react-start/flamegraph-bench/src/routes/index.tsx`:
- Line 1: The import order violates ESLint's alphabetical rule: swap the named
imports so Link appears before createFileRoute in the import statement from
'@tanstack/react-router' (i.e., change the import to import { Link,
createFileRoute } from '@tanstack/react-router') to satisfy the linter and keep
the symbols Link and createFileRoute correctly referenced.
In `@e2e/react-start/flamegraph-bench/src/routes/page`.$id.tsx:
- Line 1: The import specifiers are out of alphabetical order: change the named
imports in the top-level import from "createFileRoute, Link" to have "Link"
before "createFileRoute" so the statement reads with Link first; update the
import line that currently references createFileRoute and Link accordingly to
satisfy the ESLint rule.
In `@e2e/react-start/flamegraph-bench/tests/server.js`:
- Around line 4-13: The port and static path handling in createStartServer are
incorrect: coerce process.env.PORT to a number before using it (the port
variable / server.listen) and resolve the static asset path relative to this
module instead of the CWD for express.static; update the port assignment to
parseInt or Number(process.env.PORT || '3000') and change
express.static('../dist/client', ...) to use
path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../dist/client')
(referencing createStartServer, port, and the express.static call) so the server
listens on a TCP port and serves the correct build assets.
🧹 Nitpick comments (1)
e2e/react-start/flamegraph-bench/package.json (1)
21-21: Consider pinning@platformatic/flameto a specific version range.Using
"latest"for@platformatic/flamecan lead to non-deterministic builds and potential breakage when new versions are released. For more stable and reproducible benchmarking results, consider pinning to a specific version or version range.📌 Suggested approach
- "@platformatic/flame": "latest", + "@platformatic/flame": "^1.0.0",Note: Replace
^1.0.0with the appropriate current version.
| const port = process.env.PORT || 3000 | ||
|
|
||
| async function createStartServer() { | ||
| const server = (await import('../dist/server/server.js')).default | ||
| const nodeHandler = toNodeHandler(server.fetch) | ||
|
|
||
| const app = express() | ||
|
|
||
| // Serve static client assets (matching basic example pattern) | ||
| app.use(express.static('../dist/client', { redirect: false })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the server.js file
find . -name "server.js" -path "*/flamegraph-bench/tests/*" -type fRepository: TanStack/router
Length of output: 109
🏁 Script executed:
# Check the file contents
cat -n e2e/react-start/flamegraph-bench/tests/server.jsRepository: TanStack/router
Length of output: 994
🏁 Script executed:
# Check the module type of the file (is it ESM or CommonJS?)
head -20 e2e/react-start/flamegraph-bench/tests/server.jsRepository: TanStack/router
Length of output: 610
🏁 Script executed:
# Check package.json to understand the module context
find e2e/react-start/flamegraph-bench -name "package.json" -type f | head -1 | xargs catRepository: TanStack/router
Length of output: 954
🌐 Web query:
express.static relative path resolution behavior
💡 Result:
Short answer: express.static resolves a relative root path relative to the current working directory (process.cwd()), i.e. where you launched node — not relative to the module file. Use an absolute path (e.g. path.join(__dirname, 'public')) so resolution is deterministic. [1][2]
Details (one line each):
- express.static(root, ...) treats the root argument as the directory to serve; a string path is interpreted relative to the directory where the node process was started. [1]
- Because require() and __dirname are module-relative but process.cwd() depends on how you start the app, using path.join(__dirname, '...') or path.resolve(...) is the recommended pattern. [1][2]
Example:
- app.use('/static', express.static(path.join(__dirname, 'public'))). [1][2]
Sources:
[1] Express docs — Serving static files.
[2] StackOverflow explanation showing express.static uses the process launch directory.
🌐 Web query:
Node.js process.env.PORT string named pipe behavior
💡 Result:
Short answer: If process.env.PORT is a string that looks like a path (on Windows: begins with \?\pipe\ or \.\pipe), Node's server.listen treats it as an IPC path (named pipe) instead of a TCP port. A numeric string ("3000") is treated as a port number, but a pipe-style string is treated as an IPC path — so pass a Number (e.g. Number(process.env.PORT) or parseInt) when you mean a TCP port, or pass the pipe string when you want a named pipe. [1][2]
Example:
- TCP port (ensure numeric):
const port = Number(process.env.PORT) || 3000;
server.listen(port); - Named pipe (Windows):
const pipe = process.env.PORT; // e.g. "\\?\pipe\myapp"
server.listen(pipe);
References:
- Node.js net.server.listen (IPC / named-pipe behavior on Windows). [1]
- Node.js docs for server.listen variations (port vs path). [2]
🏁 Script executed:
# Check the vite config to understand build output
find e2e/react-start/flamegraph-bench -name "vite.config.*" -o -name "tsconfig.*" | head -5Repository: TanStack/router
Length of output: 153
🏁 Script executed:
# Look for build configuration and output pattern
find e2e/react-start/flamegraph-bench -type f \( -name "*.config.*" -o -name "*.json" \) | grep -E "(vite|build|tsconfig)" | head -10Repository: TanStack/router
Length of output: 153
🏁 Script executed:
# Check what files exist in the flamegraph-bench directory structure
ls -la e2e/react-start/flamegraph-bench/ 2>/dev/null | head -20Repository: TanStack/router
Length of output: 527
🏁 Script executed:
# Verify if there's a dist or build directory already
find e2e/react-start/flamegraph-bench -maxdepth 2 -type d -name "dist" -o -type d -name "build" 2>/dev/nullRepository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check the vite config to understand where dist is built
cat e2e/react-start/flamegraph-bench/vite.config.tsRepository: TanStack/router
Length of output: 315
🏁 Script executed:
# Also check if there's a root-level vite config
cat e2e/react-start/vite.config.ts 2>/dev/null || cat vite.config.ts 2>/dev/null || echo "No root vite config found"Repository: TanStack/router
Length of output: 84
🏁 Script executed:
# Check the actual build output by looking at how tanstackStart plugin configures output
# Let's search for dist references in the codebase to understand build structure
rg "dist/client|dist/server" e2e/react-start/flamegraph-bench/ --type ts --type js -A 2 -B 2Repository: TanStack/router
Length of output: 878
🏁 Script executed:
# Check if there are any other references to how the dist is structured in the project
rg "import.*dist" e2e/react-start/flamegraph-bench/ --type ts --type jsRepository: TanStack/router
Length of output: 175
🏁 Script executed:
# Let's trace the actual build setup by checking package.json build command
cat e2e/react-start/flamegraph-bench/package.json | grep -A 10 "scripts"Repository: TanStack/router
Length of output: 573
🏁 Script executed:
# Check if there's a monorepo root vite config or build setup
cat vite.config.ts 2>/dev/null | head -30Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Let's see what the actual directory structure shows - does a dist exist anywhere?
find e2e/react-start -maxdepth 3 -type d | grep -E "^e2e/react-start/(dist|flamegraph-bench)" | sortRepository: TanStack/router
Length of output: 211
🏁 Script executed:
# Check if the coding guidelines mention ESLint router rules for this file
cat .eslintrc* 2>/dev/null | grep -i router || echo "No ESLint config found"Repository: TanStack/router
Length of output: 81
🏁 Script executed:
# Check if there are ESLint rules configured anywhere
find . -name ".eslintrc*" -o -name "eslint.config.*" | head -5 | xargs cat 2>/dev/nullRepository: TanStack/router
Length of output: 4371
Coerce PORT to number and resolve static asset path relative to module location.
The code passes process.env.PORT as a string to server.listen(), which Node treats as an IPC pipe path on Windows instead of a TCP port. Additionally, express.static('../dist/client') resolves relative to the process working directory, so it points to the wrong location when the build output is in ./dist/client. Coerce PORT to a number and resolve the asset path relative to this file.
Proposed fix
import { toNodeHandler } from 'srvx/node'
import express from 'express'
+import path from 'node:path'
+import { fileURLToPath } from 'node:url'
-const port = process.env.PORT || 3000
+const port = Number(process.env.PORT) || 3000
+const __dirname = path.dirname(fileURLToPath(import.meta.url))
+const clientDir = path.resolve(__dirname, '../dist/client')
// Serve static client assets (matching basic example pattern)
- app.use(express.static('../dist/client', { redirect: false }))
+ app.use(express.static(clientDir, { redirect: false }))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const port = process.env.PORT || 3000 | |
| async function createStartServer() { | |
| const server = (await import('../dist/server/server.js')).default | |
| const nodeHandler = toNodeHandler(server.fetch) | |
| const app = express() | |
| // Serve static client assets (matching basic example pattern) | |
| app.use(express.static('../dist/client', { redirect: false })) | |
| import { toNodeHandler } from 'srvx/node' | |
| import express from 'express' | |
| import path from 'node:path' | |
| import { fileURLToPath } from 'node:url' | |
| const port = Number(process.env.PORT) || 3000 | |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)) | |
| const clientDir = path.resolve(__dirname, '../dist/client') | |
| async function createStartServer() { | |
| const server = (await import('../dist/server/server.js')).default | |
| const nodeHandler = toNodeHandler(server.fetch) | |
| const app = express() | |
| // Serve static client assets (matching basic example pattern) | |
| app.use(express.static(clientDir, { redirect: false })) |
🤖 Prompt for AI Agents
In `@e2e/react-start/flamegraph-bench/tests/server.js` around lines 4 - 13, The
port and static path handling in createStartServer are incorrect: coerce
process.env.PORT to a number before using it (the port variable / server.listen)
and resolve the static asset path relative to this module instead of the CWD for
express.static; update the port assignment to parseInt or
Number(process.env.PORT || '3000') and change express.static('../dist/client',
...) to use path.resolve(path.dirname(fileURLToPath(import.meta.url)),
'../dist/client') (referencing createStartServer, port, and the express.static
call) so the server listens on a TCP port and serves the correct build assets.
To run the benchmark
cd e2e/react-start/flamegraph-bench pnpm benchThis will
expressapp (tests/server.js) through@platformatic/flamefor perf measurementstests/bench.js) usingautocannonfor load testingThis results in a load test summary by autocannon
And
flamegenerates:cpu-profilein html (open in browser), in markdown (give to AI), in protobuf (open in speedscope.app)heap-profilein the same versionsSummary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.